feat(api): Rotate the calibrated module offset if the module was physically moved and rotated. #13441

vegano1 · 2023-08-31T20:07:27Z

Overview

When a module is calibrated, the generated module offset vector is valid for the current location the module is in. However, if the module is physically moved and rotated across the deck then the original calibrated offset vector is no longer valid. So we need to apply a rotation of 180 degrees to the calibrated module offset vector if the module was ever moved and rotated.

Test Plan

Make sure we can still perform a module calibration on all supported modules (temp, heater-shaker, thermocycler)
Make sure we are correctly saving/loading the calibrated module offset data
Make sure we now return the location data along with the vector offset when executing the CalibrateModule PE command over HTTP.
Make sure this does not make well positions and labware grip points less accurate
Make sure that the module calibration offset is rotated when the module is physically moved across the deck. The module calibration offset tends to be small <1mm, let's manually edit the stored calibration file in disk to get an exaggerated offset that is easy to visually see a change in.

Changelog

Added new ModuleOffsetData type to contain ModuleOffsetVector and DeckSlotLocation data.
The CalibrateModuleResult PE command now returns DeckSlotLocation in addition to moduleOffset to be used in ModuleStore.
The ModuleStore now uses ModuleOffsetData instead of ModuleOffsetVector since we want to store both the offset and the deck slot location.
The get_module_calibration_offset function now rotates the calibrated module offset by 180 degrees if the module was physically moved and rotated to a qualifying deck slot different from the original calibrated location.
Make the slot member in the ModuleCalibrationOffset class into a required field since you won't have an offset without performing a calibration.
Refactor module calibration loading mechanism so we don't need to pass in a slot arg as this is loaded from disk and will always be present.
Loading module calibration now returns Optional[ModuleCalibrationOffset] instead of a default object, as some modules just haven't been calibrated yet and we don't want to assume the slot they were calibrated in.

Review requests

I'm a bit iffy on the implementation for the _needs_rotation function as this is a fixed set of locations we use to determine if a rotation is needed or not, so it feels rather brittle. I would love some feedback on this, maybe there's a better way of doing this.

Risk assessment

Low, module calibration has not been exposed yet.

… the DeckSlotLocation a particular offset was generated in.

… and rotated.

codecov · 2023-08-31T20:12:33Z

Codecov Report

Merging #13441 (ba2bed8) into chore_release-7.0.1 (1879b24) will increase coverage by 0.04%.
Report is 9 commits behind head on chore_release-7.0.1.
The diff coverage is n/a.

Additional details and impacted files

@@                   Coverage Diff                   @@
##           chore_release-7.0.1   #13441      +/-   ##
=======================================================
+ Coverage                71.27%   71.32%   +0.04%     
=======================================================
  Files                     2427     1588     -839     
  Lines                    68280    52777   -15503     
  Branches                  7998     3443    -4555     
=======================================================
- Hits                     48669    37643   -11026     
+ Misses                   17731    14599    -3132     
+ Partials                  1880      535    -1345

Flag	Coverage Δ
g-code-testing	`96.44% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...i/src/opentrons/hardware_control/module_control.py	`72.64% <ø> (ø)`
api/src/opentrons/hardware_control/ot3api.py	`79.59% <ø> (ø)`
.../protocol_engine/resources/module_data_provider.py	`100.00% <ø> (ø)`
...pi/src/opentrons/protocol_engine/state/geometry.py	`100.00% <ø> (ø)`
api/src/opentrons/protocol_engine/state/modules.py	`91.48% <ø> (ø)`
api/src/opentrons/protocol_engine/state/state.py	`100.00% <ø> (ø)`
api/src/opentrons/protocol_engine/types.py	`97.56% <ø> (ø)`

... and 839 files with indirect coverage changes

api/src/opentrons/protocol_engine/state/modules.py

SyntaxColoring

Lookin' good.

api/src/opentrons/protocol_engine/commands/calibration/calibrate_module.py

api/src/opentrons/protocol_engine/types.py

SyntaxColoring · 2023-09-01T20:41:11Z

api/src/opentrons/protocol_engine/state/modules.py

+    A = (
+        DeckSlotName.SLOT_D1,
+        DeckSlotName.SLOT_C1,
+        DeckSlotName.SLOT_B1,
+        DeckSlotName.SLOT_A1,
+    )
+    B = (DeckSlotName.SLOT_D3, DeckSlotName.SLOT_C3, DeckSlotName.SLOT_B3)


If we do stick with this basic idea of implementing _needs_rotation() with a fixed set of locations, can we:

Change A and B to COL_1 and COL_3? "A" and "B" are confusing to me because they look like they're referring to row A and row B.

Add DeckSlotName.SLOT_A3, where the fixed trash currently is? I don't want us to forget it when the trash is made movable, and I think it should be harmless to add it now.

api/src/opentrons/protocol_engine/state/modules.py

sfoster1

Can we use the slot transform for this? That would simplify a lot of the logic. The explicit "which slots need rotation" considerations are baked into the module definitions (though they may need added entries for flex-style names).

The slot transforms in the module definitions are 3d affine transforms in encoded matrix form; you can either use them as-is by augmenting the calibration offset to 1x4 with a 0 to get only the linear-transform-part or deaugment the affine transforms back to 3d linear transforms and multiply as normal.

api/src/opentrons/protocol_engine/state/modules.py

sanni-t · 2023-09-27T20:51:03Z

Can we use the slot transform for this? That would simplify a lot of the logic. The explicit "which slots need rotation" considerations are baked into the module definitions (though they may need added entries for flex-style names).

The slot transforms in the module definitions are 3d affine transforms in encoded matrix form; you can either use them as-is by augmenting the calibration offset to 1x4 with a 0 to get only the linear-transform-part or deaugment the affine transforms back to 3d linear transforms and multiply as normal.

Discussed in person with @vegano1 and @sfoster1 : we can't use slot transforms for rotating the calibration offset since that's not what the slot transforms are used as. The slot transforms are entities that will convert the default labwareOffsetVector to one appropriate for a particular slot on a particular machine. It will include rotational components only if the default vector needs to be rotated, which is not true of all modules on the OT2 and not true of any modules on the Flex since the module caddies take care of it for the modules that can be placed in multiple locations (temperature & H/S modules).

So the decision is to apply a separate rotation to just the calibration offset if the saved calibration was done on a slot in a different column than the current location. Then this rotated calibration offset will be added to the nominal, slot transformed module offset to get the final, calibrated offset vector for a labware on the module.

remove the redundant modules.get_module_offset since the geometries view now handles a lot of that functionality

sfoster1

Looks good once we get some tests for the rotation process and logic in there!

api/src/opentrons/protocol_engine/state/geometry.py

api/src/opentrons/hardware_control/modules/module_calibration.py

sanni-t

LGTM!
As @sfoster1 mentioned, we should add a test that tests the rotation of the offsets.

api/src/opentrons/hardware_control/ot3api.py

api/src/opentrons/hardware_control/modules/module_calibration.py

api/src/opentrons/protocol_engine/state/geometry.py

api/src/opentrons/protocol_engine/state/modules.py

vegano1 added 2 commits August 30, 2023 18:06

refactor module calibration loading/saving mechanism so we can expose…

7f4fc16

… the DeckSlotLocation a particular offset was generated in.

bug(api): Rotate the calibrated module offset if the module was moved…

02e3d4f

… and rotated.

vegano1 requested review from a team as code owners August 31, 2023 20:07

vegano1 requested review from sanni-t and sfoster1 August 31, 2023 20:07

vegano1 requested a review from SyntaxColoring September 1, 2023 18:34

SyntaxColoring reviewed Sep 1, 2023

View reviewed changes

api/src/opentrons/protocol_engine/state/modules.py Outdated Show resolved Hide resolved

SyntaxColoring reviewed Sep 1, 2023

View reviewed changes

sfoster1 requested changes Sep 6, 2023

View reviewed changes

api/src/opentrons/protocol_engine/state/modules.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_engine/state/modules.py Outdated Show resolved Hide resolved

vegano1 added 4 commits September 7, 2023 14:07

Merge branch 'chore_release-7.0.0' into RCORE-820_module_offset_rotation

372b3bc

wip

ae7869c

Merge branch 'chore_release-7.0.0' into RCORE-820_module_offset_rotation

9917ade

Merge branch 'chore_release-7.0.0' into RCORE-820_module_offset_rotation

4f6cba1

sfoster1 force-pushed the chore_release-7.0.0 branch from a680b77 to b8dd3f3 Compare September 27, 2023 21:00

sfoster1 requested a review from a team as a code owner September 27, 2023 21:00

sfoster1 requested review from jerader and removed request for a team September 27, 2023 21:00

sfoster1 changed the base branch from chore_release-7.0.0 to chore_release-7.0.1 September 27, 2023 21:24

vegano1 added 3 commits September 28, 2023 14:52

clean up module calibration rotation mechanic

0fc2b8b

remove the redundant modules.get_module_offset since the geometries view now handles a lot of that functionality

Merge branch 'chore_release-7.0.1' into RCORE-820_module_offset_rotation

0df5e80

lint

04880d0

vegano1 requested review from sfoster1 and SyntaxColoring September 29, 2023 15:04

sfoster1 approved these changes Oct 2, 2023

View reviewed changes

api/src/opentrons/protocol_engine/state/geometry.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_engine/state/geometry.py Outdated Show resolved Hide resolved

api/src/opentrons/hardware_control/modules/module_calibration.py Outdated Show resolved Hide resolved

sanni-t approved these changes Oct 2, 2023

View reviewed changes

vegano1 added 2 commits October 3, 2023 11:15

address feadback and add unit tests

c1bb445

remove print

ba2bed8

vegano1 merged commit af7c4d4 into chore_release-7.0.1 Oct 4, 2023

vegano1 deleted the RCORE-820_module_offset_rotation branch October 4, 2023 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Rotate the calibrated module offset if the module was physically moved and rotated. #13441

feat(api): Rotate the calibrated module offset if the module was physically moved and rotated. #13441

vegano1 commented Aug 31, 2023 •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading

SyntaxColoring left a comment

SyntaxColoring Sep 1, 2023

sfoster1 left a comment

sanni-t commented Sep 27, 2023

sfoster1 left a comment

sanni-t left a comment

feat(api): Rotate the calibrated module offset if the module was physically moved and rotated. #13441

feat(api): Rotate the calibrated module offset if the module was physically moved and rotated. #13441

Conversation

vegano1 commented Aug 31, 2023 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Aug 31, 2023 • edited Loading

Codecov Report

SyntaxColoring left a comment

Choose a reason for hiding this comment

SyntaxColoring Sep 1, 2023

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sanni-t commented Sep 27, 2023

sfoster1 left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

vegano1 commented Aug 31, 2023 •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading